feat: add get_by_ids, reading documents back by item ID - #21
minseokpark-CL wants to merge 7 commits into
Conversation
euphoria0-0
left a comment
There was a problem hiding this comment.
integration test 한번 실행 부탁드립니다!
릴리즈 없어도 가능하고 msa에서 get_by_ids 추가한 PR에서 직접 빌드한걸로 테스트해보면 됩니다
Envector.get_by_ids(ids, /, *, partition_name=None) returns a Document for every id that names a live item, in request order. Non-item ids, unknown ids and deleted ids are left out rather than raised, as LangChain's contract asks. It uses pyenvector's Index.get_by_ids, which reads stored metadata by item_id without a search, so a document is readable as soon as add_texts returns and stops being readable as soon as delete returns. Item ids are unique within a partition only: a document added under a named partition must be read with that partition_name, otherwise the same number in the default partition is a different document. The search result parsing is shared with get_by_ids through _stored_document. has_get_by_ids is now True; three of the four standard tests pass and test_add_documents_with_existing_ids is xfail because a caller-chosen id cannot be created. Requires a pyenvector with Index.get_by_ids; the version pin is raised once that SDK release exists. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Index.get_by_ids already splits the ids at the server's per-call cap, so the wrapper's own 10,000-id loop did nothing. get_by_ids now passes all ids in one call, and the unit test for the wrapper-side split is removed; the SDK's tests cover the split. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Review on envector-msa #2565 asked that the difference be written where users read it: a document is readable by id before a merge makes it searchable, and right after an update the new content is readable while search may still score the old vector. Both are expected. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
bebe66e to
a64d191
Compare
|
@euphoria0-0 integration test 돌렸습니다. 실패 없습니다. 환경
명령 결과: 28 passed, 12 skipped, 3 xfailed (3m34s)
|
…ut it Index.get_by_ids ships in a pyenvector release that is not out yet, while this package accepts pyenvector>=1.6.2. On 1.6.2 get_by_ids failed with "'Index' object has no attribute 'get_by_ids'". Check the installed SDK first and raise NotImplementedError, LangChain's usual answer for an unsupported get_by_ids, saying to upgrade pyenvector. SDK_HAS_GET_BY_IDS drives has_get_by_ids in the standard tests and skips tests/integration_tests/test_get_by_ids.py, so the suites skip instead of failing on an older SDK. README Limitations gets one line. The pin and the README "Requires" line move once the release exists. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
get_by_ids turned ids into int(...) through the add_texts helper, so True read item 1 and 3.9 or 3.0 read item 3 — a document the caller did not ask for. pyenvector's own check rejects bool and float, but it only ever saw the already-converted int. _readable_item_id accepts a positive int or its decimal string and nothing else; any other value names no item and is left out, as LangChain's get_by_ids contract asks. Unit tests cover bool, float, "3.0", negatives, zero, blanks, None, bytes and lists. The same conversion in delete / update_* / upsert_documents and in the ids of add_texts predates this PR and is fixed in the next one. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The Limitations line on item IDs listed delete / update / upsert / add_documents but not get_by_ids, and said "any numeric id is taken to be one of them", which get_by_ids no longer does for 3.0 or 3.9. Name the accepted forms (the returned strings, or ints) and say that a non-item ID such as a UUID is never found by get_by_ids. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…tence The docstring said that right after update_documents search "may still score the old vector". It does not: a vector update deprecates the old shard slot in the same transaction that writes the new metadata, so until the new vector is searchable search leaves the document out rather than ranking it by the old vector. update_documents waits for that by default (await_update), so the gap exists only when it is called with await_completion=False. Say that instead. Also drop the list of accepted ID types from the first sentence: the IDs callers pass back are the ones add_texts and search return, and the next sentence already says anything that is not an item ID is left out. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Kim-YeongHyeon
left a comment
There was a problem hiding this comment.
이 리뷰는 AI 에이전트가 전권 위임 하에 생성했습니다. 필수 부담당자 리뷰를 대체하지 않으며,
머지 판단은 사람 리뷰어에게 있습니다.
| text = md_obj.get("text", "") if "_raw" not in md_obj else md_obj["_raw"] | ||
| metadata = md_obj.get("metadata", {}) if "_raw" not in md_obj else {} | ||
| if not text and not metadata: | ||
| doc, has_content = _stored_document(item) |
There was a problem hiding this comment.
검색 경로에서 Document 생성이 filter·score_threshold 판정보다 앞으로 옮겨져, refactor인데도 동작이 바뀝니다. 이전에는 threshold나 filter에서 걸러지던 비정상 payload가 이제는 pydantic ValidationError를 내고 검색 전체를 중단시킵니다. fake client로 score=0.1, score_threshold=0.5를 주고 확인한 결과입니다.
{"text":"x","metadata":null}: base[], headValidationError{"text":123,"metadata":{}}: base[], headValidationError
get_by_ids도 같은 함수를 쓰므로, 다른 client가 쓴 행 하나만 형식이 어긋나도 호출 전체가 예외로 끝납니다(LangChain 계약은 raise하지 않는 것입니다). _stored_document가 text가 str이 아니거나 metadata가 dict가 아니면 None을 돌려주고, 두 호출처가 그 행을 건너뛰게 하는 방법을 제안합니다.
| return value if value > 0 else None | ||
| if isinstance(value, str): | ||
| text = value.strip() | ||
| if text.isdecimal(): |
There was a problem hiding this comment.
상한이 없어서 "9223372036854775808"이 isdecimal()과 SDK의 _item_id 검사를 모두 통과하고, proto의 repeated int64 item_ids에 들어가면서 ValueError: Value out of range가 납니다. 존재할 수 없는 ID인데 빠지지 않고 예외가 납니다. item_id > 2**63 - 1이면 None을 돌려주도록 해 주십시오.
또한 isdecimal()은 "3"(전각)이나 "٣" 같은 비ASCII 숫자도 받아 item 3을 읽습니다. #22에서 이 규칙을 모든 메서드에 적용할 계획이라면 text.isascii() and text.isdigit()로 범위를 정하고 docstring에 적어 두면 좋겠습니다.
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "not_an_id", [True, False, 3.9, 3.0, "3.0", "-3", "0", " ", None, b"3", [3]] |
There was a problem hiding this comment.
문자열 "0", "-3"은 있지만 int 0, -3은 없습니다. _readable_item_id의 return value if value > 0 else None을 return value로 바꿔도 unit test 117개가 모두 통과합니다. 실제 SDK에서는 int 0이 EnvectorValidationError를 냅니다. int 0과 -3을 추가해 주십시오.
같은 맥락에서, 검색이 '{"text":"","metadata":{}}'를 건너뛰는지 확인하는 테스트도 없습니다(has_content를 항상 True로 바꿔도 통과합니다). 이 PR이 그 코드를 옮겼으므로 한 건 추가해 두면 좋겠습니다.
| metadata=metadata, | ||
| id=str(doc_id) if doc_id is not None else None, | ||
| ) | ||
| return doc, bool(text or metadata) |
There was a problem hiding this comment.
has_content는 검색 경로만 쓰고 get_by_ids는 [0]으로 버립니다. Document만 돌려주고, 검색 쪽에서 if not (doc.page_content or doc.metadata): continue로 판단하면 튜플이 없어집니다. 위 검증 문제를 고치면서 반환값을 바꾸게 되면 함께 정리할 수 있습니다.
get_by_ids추가 — item ID 로 문서 읽기(English below.)
무엇이 달라지나
Envector.get_by_ids(ids, /, *, partition_name=None)가 생긴다.add_texts/add_documents가 돌려준 item ID(또는 검색 결과의Document.id)로, 검색 없이 문서를 읽는다.Document하나를ids순서대로 돌려준다.Document.id는 item ID 다. 없는 ID, 삭제된 ID, item ID 가 아닌 값은 에러 없이 빠진다(LangChainVectorStore.get_by_ids의 규칙). item ID 는 양의int와 그 십진수 문자열뿐이고,bool·float(3.9,3.0)·"3.0"은 item ID 가 아니다 —int(...)로 바꾸면3.9가 item 3 을 읽게 되므로(_readable_item_id). 같은 ID 가 여러 번 오면 한 번만 읽는다.add_texts가 끝나자마자 읽을 수 있고,delete가 끝나자마자 읽히지 않는다. 인덱스 load 도 필요 없다. 벡터를 바꾸는update_documents를 기다리지 않고(await_completion=False) 부르면, 새 벡터가 검색 가능해질 때까지get_by_ids는 새 내용을 돌려주고 검색에서는 그 문서가 빠질 수 있다. 기본값(await_update)은 기다리므로 이 틈이 없다. docstring 에 적었다.partition_name으로 읽어야 한다.Index에get_by_ids가 없으면NotImplementedError를 내고 pyenvector 를 올리라고 알린다. LangChain 이get_by_ids를 지원하지 않는 store 에서 내는 것과 같은 예외다. 같은 판단을SDK_HAS_GET_BY_IDS로 둔다.Document로 바꾸는 코드를_stored_document로 떼어 내 검색과get_by_ids가 같이 쓴다.has_get_by_ids는SDK_HAS_GET_BY_IDS를 따른다.tests/integration_tests/test_get_by_ids.py도 같은 조건으로 skip 된다.test_add_documents_with_existing_ids는 xfail 로 표시했다 — 이 테스트는 호출하는 쪽이 정한 id("foo")로 문서를 만들 수 있다고 가정하는데, item ID 는 서버가 발급한다.get_by_ids를 넣고 받는 형식(반환된 문자열 또는int)과 "UUID 같은 ID 는get_by_ids가 찾지 못한다" 를 적음, Limitations 에서 "get_by_ids unsupported" 를 빼고 partition 안내 한 줄과 "Index.get_by_ids가 있는 pyenvector 가 필요하고, 그 전 버전에서는NotImplementedError" 한 줄, "Fetch by ID" 예제.의존성
get_by_ids는 pyenvector 의Index.get_by_ids를 부른다. 이 메서드는 CryptoLabInc/envector-msa#2565 에서 들어갔고, 그 PR 은 2026-09-30 에 merge 됐다.Index.get_by_ids가 없다:1.6.2,1.6.3-rc.1,1.6.3-rc.2모두 확인했다. PyPI 의 최신도1.6.2다.pyproject.toml과 README 의 요구 버전은pyenvector >= 1.6.2그대로다. 지금 설치하면get_by_ids호출은NotImplementedError로 끝나고, 나머지 기능은 그대로 동작한다.알려진 문제 — 다음 PR 에서 수정
delete,update_metadata,update_documents,upsert_documents와add_texts/add_documents의ids는 ID 를int(...)로 바로 바꾼다. 그래서True는 item 1,3.9와3.0은 item 3 이 된다. 예를 들어delete(ids=[3.9])는 item 3 을 지우고,add_texts(..., ids=[3.9])는 item 3 을 덮어쓴다.int와 그 십진수 문자열만 ID 로 보고bool과float는 ID 로 보지 않는다. 판별에 걸린 값의 처리는 메서드별 기존 방식을 따른다 —delete/update_*/upsert_documents는ValueError,add_texts의ids는 새 행으로 삽입하고UserWarning.get_by_ids추가와 분리해서, 기존 메서드의 동작 변경만 따로 리뷰할 수 있게 하려는 것이다.검증
python -m pytest tests -m "not integration"—35bfa33에서 117 passed. 새 unit test: 요청 순서, 없는/item ID 가 아닌 ID 생략, 삭제된 문서 제외, partition 구분, 내용이 빈 live row, 복호화된 dict payload,text가null인 envelope, 인덱스를 load 하지 않는 것,get_by_ids가 없는 SDK 에서NotImplementedError,bool·float·"3.0"·음수·0·공백·None·bytes·list 가 빠지고 SDK 호출도 없는 것(11가지),[3, " 2 ", "1", 3.9, True]에서 3·2·1 만 읽는 것.1.6.2에서SDK_HAS_GET_BY_IDS는False이고,tests/integration_tests/test_get_by_ids.py의 4개는 "installed pyenvector has no Index.get_by_ids" 로 skip 된다(652e876, 서버 없이 확인).Add
get_by_ids— read documents back by item IDWhat changes
Envector.get_by_ids(ids, /, *, partition_name=None): reads documents by the item IDsadd_texts/add_documentsreturn (or a search result'sDocument.id), without a search.Documentper live item, in the order ofids, with the item ID asDocument.id. Unknown IDs, deleted IDs and values that are not item IDs are left out rather than raised, as LangChain'sVectorStore.get_by_idscontract asks. An item ID is a positiveintor its decimal string only;bool,float(3.9,3.0) and"3.0"are not — converting them withint(...)would make3.9read item 3 (_readable_item_id). Repeated IDs are read once.add_textsreturns and stops being readable as soon asdeletereturns. The index does not need to be loaded. After anupdate_documentsthat replaces the vector and is not awaited (await_completion=False),get_by_idsreturns the new content while search may leave the document out until the new vector is searchable; the default (await_update) waits, so there is no gap. The docstring says so.partition_name.Indexhas noget_by_ids, it raisesNotImplementedErrorsaying to upgrade pyenvector — the exception LangChain raises from a store withoutget_by_ids. The same check is exposed asSDK_HAS_GET_BY_IDS.Documentmoves to_stored_document, shared by search andget_by_ids.has_get_by_idsfollowsSDK_HAS_GET_BY_IDS.tests/integration_tests/test_get_by_ids.pyskips on the same condition.test_add_documents_with_existing_idsis marked xfail — it assumes a caller-chosen id ("foo") can be created, while item IDs are issued by the server.get_by_ids, names the accepted forms (the returned strings, orints) and saysget_by_idsnever finds an ID such as a UUID; Limitations drops "get_by_ids unsupported" and gains one line on partitions and one line sayingget_by_idsneeds a pyenvector withIndex.get_by_idsand raisesNotImplementedErrorwith an earlier one; a "Fetch by ID" example.Dependency
get_by_idscalls pyenvector'sIndex.get_by_ids, added in CryptoLabInc/envector-msa#2565, which was merged on 2026-09-30.Index.get_by_ids: checked1.6.2,1.6.3-rc.1and1.6.3-rc.2. The latest on PyPI is also1.6.2.pyproject.tomland the README still requirepyenvector >= 1.6.2; installed today, aget_by_idscall ends inNotImplementedErrorand everything else works as before.Known issue — fixed in the next PR
delete,update_metadata,update_documents,upsert_documentsand theidsofadd_texts/add_documentsturn an ID intoint(...)directly, soTruebecomes item 1 and3.9or3.0becomes item 3. For example,delete(ids=[3.9])deletes item 3, andadd_texts(..., ids=[3.9])overwrites item 3.intor its decimal string is an ID;boolandfloatare not. What happens to a rejected value follows each method's existing behaviour:ValueErrorfromdelete/update_*/upsert_documents, and a new row plus aUserWarningfor theidsofadd_texts.get_by_ids.Verification
python -m pytest tests -m "not integration"— 117 passed on35bfa33. New unit tests: request order, unknown and non-item IDs left out, deleted documents excluded, partitions, a live row with empty content, a decrypted dict payload, an envelope whosetextisnull, no index load,NotImplementedErroron an SDK withoutget_by_ids,bool/float/"3.0"/ negatives / zero / blank /None/ bytes / list left out with no SDK call (11 cases), and[3, " 2 ", "1", 3.9, True]reading only 3, 2, 1.1.6.2,SDK_HAS_GET_BY_IDSisFalseand the 4 tests intests/integration_tests/test_get_by_ids.pyskip with "installed pyenvector has no Index.get_by_ids" (652e876, checked without a server).🤖 Generated with Claude Code